Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@router/core/router.go`:
- Around line 804-821: Currently if r.tlsConfig.ClientAuth.Required is true but
ClientAuth.CertFile is empty the code falls back to NoClientCert; change the
logic in the TLS setup (the block using r.tlsConfig.ClientAuth.CertFile,
caCertPool and clientAuthMode) to validate that when
r.tlsConfig.ClientAuth.Required is true there is a non-empty CertFile and a
successfully loaded/parsed CA (caCertPool), otherwise return a clear error; only
set clientAuthMode to tls.RequireAndVerifyClientCert or
tls.VerifyClientCertIfGiven after confirming the CA was loaded, and never
silently use tls.NoClientCert when Required is true.

---

Nitpick comments:
In `@router/core/router.go`:
- Around line 832-836: The TLS server config returned from the function
constructs a tls.Config without an explicit MinVersion; update the returned
tls.Config (the struct created where you set ClientCAs, Certificates and
ClientAuth) to also set MinVersion to tls.VersionTLS12 to make the minimum TLS
version explicit. Modify the tls.Config literal in router.go (where the function
returns &tls.Config{...}) to include MinVersion: tls.VersionTLS12 so the intent
is clear and future Go default changes won’t affect your server.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@router/core/router_config.go`:
- Around line 260-261: The usage map currently sets "tls_client" based only on
c.tls != nil which overreports client TLS; change it to detect actual client TLS
settings by checking the real client TLS sub-structure and its meaningful fields
(e.g. replace c.tls != nil with a check like c.tls.Client != nil &&
(c.tls.Client.CA != "" || c.tls.Client.Cert != "" || c.tls.Client.Key != "" ||
c.tls.Client.InsecureSkipVerify || c.tls.Client.ServerName != "" ) or call an
existing helper like HasClientTLS() if present) so that usage["tls_client"]
reflects whether client TLS is actually configured rather than just the wrapper
being allocated.

In `@router/core/router.go`:
- Around line 2378-2381: WithTLSConfig currently replaces r.tls wholesale and
can drop configured client branches (e.g., r.tls.Client.Subgraphs.HTTP) set
earlier by WithSubgraphTLSConfiguration in newRouter; change WithTLSConfig to
merge the incoming cfg into the existing r.tls (preserving non-nil subtrees like
r.tls.Client and r.tls.Client.Subgraphs.HTTP) instead of assigning it, or
alternatively consolidate TLS option handling so server/client TLS aren't split
across separate options; update the Option function returned by WithTLSConfig
(the closure that mutates *Router) to perform a field-wise merge/overwrite only
for non-nil fields, referencing Router.r.tls and the TlsConfig.Client/Subgraphs
structures to ensure existing client mTLS entries remain intact.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@router-tests/testenv/testenv.go`:
- Around line 725-728: The helper currently calls log.Fatal after os.ReadFile
when loading cfg.TLSConfig.Server.HTTP.Settings.CertFile (caCert, err :=
os.ReadFile(...)); replace the process-killing log.Fatal(err) with a test
assertion like require.NoError(t, err) so the current test fails instead of
terminating the whole suite, and ensure the helper has access to *testing.T (add
a t parameter if needed) and the testify/require import; make the same change
for the other occurrence around the caCert read at lines 1156-1159.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@router/core/router.go`:
- Around line 159-165: The struct HTTPServerTLSConfig exposes a derived
*tls.Config as Config which should be unexported to prevent callers from
constructing/mutating inconsistent state; change the exported field to an
unexported config *tls.Config (or remove it from the public struct entirely) and
update internal consumers (Start, NewServer and any functions that currently
read HTTPServerTLSConfig.Config) to access the package-internal compiled config
instead or use a package-level accessor that returns a safe copy; ensure any
public constructors/validators accept Settings and produce the internal compiled
config so callers never directly hold the mutable *tls.Config.

---

Nitpick comments:
In `@router-tests/testenv/testenv.go`:
- Around line 720-749: Extract the duplicated TLS client bootstrap into a single
helper, e.g. newTLSHTTPClientFromConfig(cfg *Config) (*http.Client, error), that
performs tls.LoadX509KeyPair, os.ReadFile, builds the x509.CertPool, creates
cleanhttp.DefaultPooledClient(), sets TLSClientConfig with RootCAs and
Certificates, and wraps it with retryablehttp (respecting cfg.NoRetryClient and
Retry settings) returning the final *http.Client or an error; then replace the
duplicated blocks in CreateTestEnv and CreateTestSupervisorEnv with a call to
this helper and handle the returned error consistently (either return it up the
stack or assert/require in the same manner in both functions) so both branches
remain in lockstep.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@router-tests/security/tls_test.go`:
- Around line 23-33: The shared variable testdataCertsServerTLSConfig is being
reused by parallel subtests causing concurrent mutation of
r.tls.Server.HTTP.Config; add a factory function
newTestdataCertsServerTLSConfig() that returns a fresh *core.TlsConfig for each
test (allocate/copy the testdataCertsServerTLSConfig data into a new instance)
and replace all usages of &testdataCertsServerTLSConfig with calls to
newTestdataCertsServerTLSConfig(); ensure the router initialization that mutates
r.tls.Server.HTTP.Config (via r.serverHTTPTLSConfig()) operates on the new
instance so each subtest gets an independent TLS config.

---

Nitpick comments:
In `@router-tests/security/tls_test.go`:
- Around line 189-203: Extract a test helper that builds the nested TLS config
to avoid repeating the long literal: add a function like
newServerMTLSConfig(required bool, certFile string) that returns *core.TlsConfig
(or the nested Server->HTTP->Settings structure) and internally constructs
core.TlsConfig -> core.ServerTLSConfig -> core.HTTPServerTLSConfig ->
core.HTTPServerTLSConfigSettings with ClientAuth set to
&core.HTTPServerMTLSConfigSettings{Required: required, CertFile: certFile} and
common CertFile/KeyFile defaults; then replace the repeated literal blocks
passed to testenv.Run (the TLSConfig argument) with calls to
newServerMTLSConfig(true, "../testdata/tls/cert.pem") or
newServerMTLSConfig(false, "...") as appropriate for the occurrences around the
test cases (the blocks creating core.TlsConfig / core.ServerTLSConfig /
core.HTTPServerTLSConfig / core.HTTPServerTLSConfigSettings /
core.HTTPServerMTLSConfigSettings).